-
Notifications
You must be signed in to change notification settings - Fork 431
Add pull request template with testing checklist #1971
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
rajathagasthya
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
.github/PULL_REQUEST_TEMPLATE.md
Outdated
| <!-- How was this tested? e.g., unit tests, e2e tests, manual testing on cluster --> | ||
|
|
||
| - [ ] Additional unit tests (`make test`) | ||
| - [ ] E2E tests (describe below) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure what this means. The E2E tests are written by us, so when we ask PR contributors to elaborate further, are we assuming that they've written their own E2E tests?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I originally meant this field for if users add their own E2E tests, but this is an unlikely scenario. I added an "Other" field along with "N/A".
b44ef17 to
6a4a266
Compare
c8d343e to
5506b9b
Compare
.github/PULL_REQUEST_TEMPLATE.md
Outdated
|
|
||
| <!-- Brief description of the change --> | ||
|
|
||
| ## Context (optional) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we need a separate section. Description can include the background or set the context of a change that a contributor intends to make
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated.
.github/PULL_REQUEST_TEMPLATE.md
Outdated
|
|
||
| <!-- How was this tested? e.g., unit tests, e2e tests, manual testing on cluster --> | ||
|
|
||
| - [ ] Unit tests (`make test`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| - [ ] Unit tests (`make test`) | |
| - [ ] Unit tests (`make coverage`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
make coverage is the superset of make test
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated.
.github/PULL_REQUEST_TEMPLATE.md
Outdated
|
|
||
| - [ ] No secrets, sensitive information, or unrelated changes | ||
| - [ ] Code style followed (`make lint`) | ||
| - [ ] Tests/documentation updated if applicable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is already covered by the section below
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated.
.github/PULL_REQUEST_TEMPLATE.md
Outdated
| ## Checklist | ||
|
|
||
| - [ ] No secrets, sensitive information, or unrelated changes | ||
| - [ ] Code style followed (`make lint`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| - [ ] Code style followed (`make lint`) | |
| - [ ] Lint checks passing (`make lint`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated.
|
Under Checklist, let's add the following as well:
|
Signed-off-by: Karthik Vetrivel <kvetrivel@nvidia.com>
5506b9b to
54b377b
Compare
Description
Add a PR template to ensure contributors document how changes were tested and for general documentation purposes.
Context (optional)
We currently have no PR template. This makes it easy for PRs to slip through without adequate testing documentation.
Testing
make test)Test details:
Adding GitHub config file only - no code changes.